Skip to content

fix: correct helper pixel detection #32928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

fix: correct helper pixel detection #32928

wants to merge 5 commits into from

Conversation

@weillercarvalho
Copy link

@weillercarvalho weillercarvalho commented Nov 11, 2025 •
edited by cursor bot
Loading

Additional details

  • Fixes hasHelperPixels() so it samples the real right/bottom helper pixels (the previous off-by-one coordinates caused Jimp to
    fall back to (0,0), allowing cy.screenshot to accept captures before the runner UI was fully hidden or visible).
  • Updates packages/server/test/unit/screenshots_spec.js to expect the corrected coordinates and adds a regression test that fails
    if getPixelColor reads outside [0, width-1] × [0, height-1], preventing the bug from regressing.

Steps to test

  1. From the repo root run yarn workspace @packages/server test test/unit/screenshots_spec.js.
  2. Observe that the helper-pixel tests pass. (On WSL/OneDrive the suite stops early with “no browsers found in machineBrowsers”;
    on macOS/Linux with Chrome/Edge installed the spec completes.)

How has the user experience changed?

Before: cy.screenshot({ capture: 'app' | 'fullPage' }) could finish while the runner UI was still visible because the helper-
pixel detector always read (0,0).
After: the detector clamps the coordinates, so screenshots aren’t accepted until the helper pixels truly disappear/appear.

PR Tasks

Note

Clamp helper-pixel sampling to valid image bounds in screenshot detection and add tests; update changelog.

  • Server (screenshots):
    • Clamp helper-pixel reads in hasHelperPixels() to [0..width-1] × [0..height-1] via getClampedPixel, preventing out-of-bounds getPixelColor calls in packages/server/lib/screenshots.ts.
    • Adjust pixel coordinate usage to use maxX/maxY for right/bottom corners.
  • Tests:
    • Update packages/server/test/unit/screenshots_spec.js to stub getPixelColor on the image instance; add regression test asserting coordinates stay within bitmap bounds.
    • Provide deterministic browsers in test setup and restore in teardown.
  • Changelog:
    • Add bugfix entry noting corrected helper pixel bounds and related unit test error.

Written by Cursor Bugbot for commit 571d285. This will update automatically on new commits. Configure here.

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

cypress-app-bot commented Nov 11, 2025

@jennifer-shehane jennifer-shehane changed the title fix:correct helper pixel detection fix: correct helper pixel detection Nov 11, 2025
@jennifer-shehane
Copy link
Member

jennifer-shehane commented Nov 11, 2025

@weillercarvalho Very interesting find. I will run the tests to see if they pass.

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Nov 11, 2025

@weillercarvalho There is a failure here you can see in CircleCI:

  1) lib/screenshots .capture does not read helper pixels outside the bitmap bounds:
     TypeError: this.getPixelColor.restore is not a function
      at Context.<anonymous> (test/unit/screenshots_spec.js:165:26)
      at processImmediate (node:internal/timers:485:21)

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Nov 11, 2025

@weillercarvalho It might be helpful to know that our jimp version currently in this repo is 0.22.12.

@weillercarvalho
Copy link
Author

weillercarvalho commented Nov 12, 2025

@jennifer-shehane Thank you for the clarification. I will look into the error, try to correct it, and commit later.

@weillercarvalho
Copy link
Author

weillercarvalho commented Nov 12, 2025 •
edited
Loading

@jennifer-shehane I fixed the failing spec by making fake Jimp image behave the way the real library does: it now defines a getPixelColor method and we stub that method with Sinon, so the new “helper pixels stay in-bounds” test can call .restore() without error. I also stubbed ctx._apis.browserApi.getBrowsers in the suite’s before/after hooks to return a deterministic Chromium entry; that keeps the DataContext from throwing “no browsers found” when the test environment can’t detect real browsers, ensuring the updated helper-pixel logic and its regression test both run cleanly.

Also all unit tests passed as well, so now I hope that everything its okay.
Captura de tela 2025-11-11 222117

Captura de tela 2025-11-11 222132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jennifer-shehane jennifer-shehane Awaiting requested review from jennifer-shehane

@cacieprins cacieprins Awaiting requested review from cacieprins

At least 1 approving review is required to merge this pull request.

Assignees

@weillercarvalho weillercarvalho

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cy.screenshot helper pixel check uses out-of-bounds coordinates

4 participants

@weillercarvalho @CLAassistant @cypress-app-bot @jennifer-shehane